Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add filter:cgolib #1277

Closed
wants to merge 5 commits into from
Closed

Add filter:cgolib #1277

wants to merge 5 commits into from

Conversation

llhhbc
Copy link

@llhhbc llhhbc commented Apr 15, 2019

Add a new filter: cgolib.

In this filter, It will call go lib filter by open dynamic lib.

Signed-off-by: lonthui.li [email protected]

@llhhbc llhhbc changed the base branch from master to 1.0 April 15, 2019 06:40
@cosmo0920
Copy link
Contributor

@llhhbc Could you send your PR into master branch (v1.1 developing branch)?

plugins/filter_cgolib/cgolib.c Outdated Show resolved Hide resolved
@llhhbc llhhbc changed the base branch from 1.0 to master April 15, 2019 07:05
@llhhbc llhhbc changed the base branch from master to 1.0 April 15, 2019 07:06
@llhhbc llhhbc changed the base branch from 1.0 to master April 15, 2019 07:07
@llhhbc llhhbc force-pushed the addcgolib branch 2 times, most recently from de308c7 to 23ce80f Compare April 15, 2019 07:29
@llhhbc
Copy link
Author

llhhbc commented Apr 15, 2019

@cosmo0920 I have rebase to master branch, and commit again.

@llhhbc llhhbc force-pushed the addcgolib branch 2 times, most recently from 4d3a3d0 to e1c104e Compare April 15, 2019 09:43
@llhhbc
Copy link
Author

llhhbc commented Apr 21, 2019

@cosmo0920 Can it be merge?

@cosmo0920
Copy link
Contributor

@llhhbc Please ask @edsiper to review this PR.

@llhhbc
Copy link
Author

llhhbc commented Apr 22, 2019

@edsiper Can it be merge?

@edsiper
Copy link
Member

edsiper commented Apr 26, 2019

to be reviewed by @wfernandes @jasonkeene

Copy link
Contributor

@cosmo0920 cosmo0920 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm concerning about exporting function symbol naming rule.

On Output Golang proxy, we use CamelCase names for them.

How about using the following exporting function names?

  • Golib_init -> GoLibInit
  • Golib_filter -> GoLibFilter
  • Golib_exit -> GoLibExit

I've added some nitpicks comments, but other implementation looks good to me.

plugins/filter_cgolib/cgolib.c Outdated Show resolved Hide resolved
plugins/filter_cgolib/cgolib.c Outdated Show resolved Hide resolved
@edsiper
Copy link
Member

edsiper commented May 3, 2019

@wfernandes @jasonkeene ^

@wfernandes
Copy link
Member

@edsiper Apologies for the delay. We'll take a look at it soon.

@llhhbc
Copy link
Author

llhhbc commented May 16, 2019

I'm concerning about exporting function symbol naming rule.

On Output Golang proxy, we use CamelCase names for them.

How about using the following exporting function names?

  • Golib_init -> GoLibInit
  • Golib_filter -> GoLibFilter
  • Golib_exit -> GoLibExit

I've added some nitpicks comments, but other implementation looks good to me.

It is fixed.

@llhhbc llhhbc closed this May 16, 2019
@llhhbc llhhbc reopened this May 16, 2019
@llhhbc
Copy link
Author

llhhbc commented May 16, 2019

@cosmo0920 @wfernandes @jasonkeene Please check again.

plugins/filter_cgolib/Readme.md Outdated Show resolved Hide resolved
plugins/filter_cgolib/Readme.md Outdated Show resolved Hide resolved
plugins/filter_cgolib/Readme.md Outdated Show resolved Hide resolved
plugins/filter_cgolib/cgolib.c Outdated Show resolved Hide resolved
@llhhbc
Copy link
Author

llhhbc commented Jan 9, 2021

Any timelines on when this will be complete? I'm about to start work on an log encryption filter in Lua but being able to write it in Go would be ideal.

Cheers!

I will update this filter now

@llhhbc
Copy link
Author

llhhbc commented Jan 9, 2021

@edsiper I have change code

Signed-off-by: longhui.li <[email protected]>
Copy link
Contributor

@cosmo0920 cosmo0920 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be honest, I feel that this PR is unnatural because auto-generated cgo.h header is committed by hand.
There's something wrong for me.

plugins/filter_go/cgo.h Show resolved Hide resolved
@llhhbc llhhbc requested a review from cosmo0920 June 13, 2021 04:12
@cosmo0920
Copy link
Contributor

@llhhbc Could you rebase off master? Current your branch is not able to build with the recent gcc compilers due to duplicated symbols.

@llhhbc
Copy link
Author

llhhbc commented Jun 14, 2021

@llhhbc Could you rebase off master? Current your branch is not able to build with the recent gcc compilers due to duplicated symbols.

OK

@llhhbc
Copy link
Author

llhhbc commented Jun 14, 2021

@cosmo0920 I have merge the master.

@llhhbc
Copy link
Author

llhhbc commented Aug 20, 2021

@edsiper
Copy link
Member

edsiper commented Sep 7, 2021

@nokute78 please manage the review/cleanup/merge of this PR

@nokute78
Copy link
Collaborator

nokute78 commented Sep 26, 2021

How about passing raw message pack between Go and C ?
Fluent-bit-go supports messagepack decoder.
https://github.com/fluent/fluent-bit-go/blob/master/output/decoder.go
(By the way , we need an encoder to create filtered message pack data..)

Output plugin of C passes raw messagepack.

int proxy_go_flush(struct flb_plugin_proxy_context *ctx,

Currently, only string type is supported.
I think it is better to support number and other primitive data types.

@llhhbc
Copy link
Author

llhhbc commented Mar 8, 2022

How about passing raw message pack between Go and C ? Fluent-bit-go supports messagepack decoder. https://github.com/fluent/fluent-bit-go/blob/master/output/decoder.go (By the way , we need an encoder to create filtered message pack data..)

Output plugin of C passes raw messagepack.

int proxy_go_flush(struct flb_plugin_proxy_context *ctx,

Currently, only string type is supported. I think it is better to support number and other primitive data types.
@nokute78 I'll try it out.

Copy link
Contributor

@cosmo0920 cosmo0920 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd considered about this once again.
This implementation should be integrated into Fluent Bit Golang plugin mechanism like as:

And filter Golang plugin can be integrated like as (without threaded mechanism):

#5056

@llhhbc
Copy link
Author

llhhbc commented Oct 29, 2022

@cosmo0920 What I need is not the output implemented with go, but the filter implemented with go. At present, fluentbit is fully supported in input and output, but the functions of filter and parse are insufficient, and it is very inconvenient to debug. The cgofilter I implemented has been online and has been running stably for 2 years, and it is also very convenient to extend it with go (because not most people can use c proficiently)

@llhhbc
Copy link
Author

llhhbc commented Oct 29, 2022

How about passing raw message pack between Go and C ? Fluent-bit-go supports messagepack decoder. https://github.com/fluent/fluent-bit-go/blob/master/output/decoder.go (By the way , we need an encoder to create filtered message pack data..)

Output plugin of C passes raw messagepack.

int proxy_go_flush(struct flb_plugin_proxy_context *ctx,

Currently, only string type is supported. I think it is better to support number and other primitive data types.

@nokute78 I'm not sure if this MR can be submitted. Because c and go, the parameters are currently passed through memory sharing, and after use, string is enough to use (because other types, the basic components of fluentbit can be solved), using msgpack has also been considered before, but it may be The cost is a bit big. i can try

@cosmo0920
Copy link
Contributor

cosmo0920 commented Feb 26, 2023

@cosmo0920 What I need is not the output implemented with go, but the filter implemented with go. At present, fluentbit is fully supported in input and output,

Now, fluent-bit supports Wasm filter instead of Golang filter. Your pull requested feature collides our filter Wasm feature.
I'm not sure that your filter plugin written in Go can be migrated as Wasm filter.
Could you confirm your filter Golang can be replaced with our implemented Wasm filter feature? Thanks.

@edsiper
Copy link
Member

edsiper commented Aug 6, 2024

we will keep the Wasm as a preference for now. I appreciate the effort here, however I think its time to close it

@edsiper edsiper closed this Aug 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs-required waiting-for-user Waiting for more information, tests or requested changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants